Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

pkg/machine/e2e: Remove unnecessary copy of machine image. #23068

Merged
merged 1 commit into from
Jun 24, 2024
Merged

pkg/machine/e2e: Remove unnecessary copy of machine image. #23068

merged 1 commit into from
Jun 24, 2024

Conversation

ashley-cui
Copy link
Member

@ashley-cui ashley-cui commented Jun 21, 2024 •
edited
Loading

Stop copying the pre-pulled uncompressed machine disk into the individual test dir. The machine pull code already makes a copy of the disk into the test's HOMEDIR/.local/share/containers/podman/machine, and works off that copy.

Before the change: TESTDIR/<image> is copied to TESTDIR/podman_test/<image> by the test, and then podman machine copies the image to TESTDIR/podman_test/.local/share/containers/podman/machine/provider/<image>

After the change: TESTDIR/<image> is copied to TESTDIR/podman_test/.local/share/containers/podman/machine/provider/<image> by podman machine

The image that is actually run is at TESTDIR/podman_test/.local/share/containers/podman/machine/provider/<image> in both instances.

Does this PR introduce a user-facing change?

None

@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None labels Jun 21, 2024
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Jun 21, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ashley-cui

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. release-note-none and removed do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None labels Jun 21, 2024
@ashley-cui
Copy link
Member Author

Initial local run of one test file, but lets see the performance in the full CI run.

Before change:
make localmachine FOCUS_FILE=basic_test.go 53.08s user 41.46s system 32% cpu 4:51.64 total

After change:
make localmachine FOCUS_FILE=basic_test.go 36.01s user 19.68s system 28% cpu 3:17.05 total

@Luap99
Copy link
Member

Luap99 commented Jun 21, 2024

Me wonders why bud ... tests are triggered on this PR. I would have assumed my new skip logic to skip bud here, i.e. on #23064 I see both machine and bud skipped so I assumed it works.

This PR touches machine so running machine test is correct but why would it trigger bud here?!

@Luap99
Copy link
Member

Luap99 commented Jun 21, 2024

Comparing to https://cirrus-ci.com/build/5300635074035712 it doesn't look like it made a difference for linux.
However macos was almost 60 mins there and 50 mins in your machine reset PR (#22941), here it is 34 mins so that looks like a very good speed up. I have not looked at macos timings much so I don't know the run to run variance but it looks like a good start.

@edsantiago
Copy link
Member

This PR touches machine so running machine test is correct but why would it trigger bud here?!

Base is very very old, does not include your changes

@Luap99
Copy link
Member

Luap99 commented Jun 21, 2024 •
edited
Loading

This PR touches machine so running machine test is correct but why would it trigger bud here?!

Base is very very old, does not include your changes

Well then I have a bug report for you :) Your logformatter base commit is showing the wrong commit then, i.e. https://api.cirrus-ci.com/v1/artifact/task/5807831486562304/html/sys-remote-rawhide-root-host-sqlite.log.html lists 9ffac33 as base. Unless I am misunderstanding what "base" means. For me it would be last commit on the branch (PR) that is already in main.

@ashley-cui
pkg/machine/e2e: Remove unnecessary copy of machine image.
4dc98e3 
Stop copying the pre-pulled uncompressed machine disk into the individual test dir. The machine pull code already makes a copy of the disk into the test's HOMEDIR/.local/share/containers/podman/machine, and works off that copy.

Before the change: TESTDIR/<image> is copied to TESTDIR/podman_test/<image> by the test, and then podman machine copies the image to TESTDIR/podman_test/.local/share/containers/podman/machine/provider/<image>

After the change: TESTDIR/<image> is copied to TESTDIR/podman_test/.local/share/containers/podman/machine/provider/<image> by podman machine

The image that is actually run is at TESTDIR/podman_test/.local/share/containers/podman/machine/provider/<image> in both instances.

Signed-off-by: Ashley Cui <[email protected]>
@ashley-cui
Copy link
Member Author

Rebased against main, and going to look at the test run from that. If the speed gains are lasting then I'll mark as ready for review :)

@Luap99
Copy link
Member

Luap99 commented Jun 21, 2024

And now the skips are working, as this only touches machine test code no sys/int/bud tests triggered.

@ashley-cui
Copy link
Member Author

Mac test is extremely flakey, but I can't think of a reason why this change would do that, going to dig a little .

@edsantiago
Copy link
Member

Machine flake is #22551

@ashley-cui ashley-cui marked this pull request as ready for review June 24, 2024 12:34
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 24, 2024
@ashley-cui
Copy link
Member Author

ashley-cui commented Jun 24, 2024 •
edited
Loading

HyperV: 10 min speedup
Linux: 1-5 min speedup
Mac: 20-30 min speedup
WSL: 1-5 min speedup

Looks like it sped up the Mac tests a lot, which makes sense, since those machines were still slightly io bound.
Ready for review!

@Luap99
Copy link
Member

Luap99 commented Jun 24, 2024

HyperV: 10 min speedup Linux: 1-5 min speedup Mac: 20-30 min speedup WSL: 1-5 min speedup

Looks like it sped up the Mac tests a lot, which makes sense, since those machines were still slightly io bound. Ready for review!

At the least for the other PRs I checked I do not see that much speed up on linux/wsl. There is a lot of variance in the run two run timings so it is hard to tell of course. Of course this is a good to start but we have to get macos/hyperv to around 20 min to achieve our 30 min overall CI time goal.

Anyhow LGTM of course

@rhatdan
Copy link
Member

rhatdan commented Jun 24, 2024

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 24, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit fe18a87 into containers:main Jun 24, 2024
90 checks passed
@Luap99 Luap99 mentioned this pull request Jun 24, 2024
Merged
@edsantiago edsantiago mentioned this pull request Jun 25, 2024
Closed
@Luap99 Luap99 mentioned this pull request Jul 2, 2024
Merged
@edsantiago edsantiago mentioned this pull request Jul 2, 2024
Open
@Luap99 Luap99 mentioned this pull request Jul 2, 2024
Merged
@stale-locking-app stale-locking-app bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 23, 2024
@stale-locking-app stale-locking-app bot locked as resolved and limited conversation to collaborators Sep 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@rhatdan rhatdan

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. machine release-note-none
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ashley-cui @Luap99 @edsantiago @rhatdan